Skip to content

Conversation

Jack251970
Copy link
Member

@Jack251970 Jack251970 commented Sep 29, 2025

CHANGES

  • Use Binding instead of event handler to handle UI logic.
  • Use Flow.Launcher.Localization to improve code quality.
  • Fix IsEnabled logic: When command type is RunCommand, these two options cannot take effect - Close Command Prompt after pressing any key & Do not close Command Prompt after command execution - so we should disabled these two options.
  • Fix ShowOnlyMostUsedCMDsNumber default value issue: Since the old Settings class set default value of ShowOnlyMostUsedCMDsNumber to 0 which is a wrong value, we need to fix it here to make sure the default value is 5

TEST

Setting panel can work well

@prlabeler prlabeler bot added bug Something isn't working Code Refactor enhancement New feature or request labels Sep 29, 2025
@github-actions github-actions bot added this to the 2.1.0 milestone Sep 29, 2025
@Jack251970 Jack251970 requested a review from Copilot September 29, 2025 15:16
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Refactors the ShellSettings UI component to use modern WPF data binding patterns instead of event handlers, and fixes the IsEnabled logic for shell options that don't apply to RunCommand.

  • Replaces event-driven UI updates with MVVM pattern using data binding
  • Implements proper property change notifications in Settings class
  • Adds converter to handle conditional enabling of shell options based on command type

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
ShellSetting.xaml.cs Replaces old event-based code-behind with clean ViewModel instantiation
ShellSetting.xaml Updates XAML to use data binding instead of event handlers and adds converter for conditional enabling
ShellSettingViewModel.cs New ViewModel implementing proper property binding and mutual exclusion logic
Settings.cs Converts auto-properties to full properties with change notifications and adds localization attributes
Main.cs Adds namespace import for Views
LeaveShellOpenOrCloseShellAfterPressEnabledConverter.cs New converter for determining when shell options should be enabled

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

gitstream-cm bot commented Sep 29, 2025

🥷 Code experts: jjw24

Jack251970, jjw24 have most 👩‍💻 activity in the files.
jjw24, Jack251970 have most 🧠 knowledge in the files.

See details

Plugins/Flow.Launcher.Plugin.Shell/Main.cs

Activity based on git-commit:

Jack251970 jjw24
SEP 26 additions & 26 deletions
AUG
JUL 8 additions & 6 deletions 28 additions & 8 deletions
JUN 107 additions & 80 deletions 74 additions & 67 deletions
MAY 6 additions & 3 deletions
APR 34 additions & 29 deletions

Knowledge based on git-blame:
Jack251970: 31%
jjw24: 11%

Plugins/Flow.Launcher.Plugin.Shell/Settings.cs

Activity based on git-commit:

Jack251970 jjw24
SEP 4 additions & 8 deletions
AUG
JUL
JUN
MAY
APR

Knowledge based on git-blame:
jjw24: 22%
Jack251970: 10%

✨ Comment /gs review for LinearB AI review. Learn how to automate it here.

Copy link

gitstream-cm bot commented Sep 29, 2025

Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX.

Copy link
Contributor

coderabbitai bot commented Sep 29, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Refactors the Shell plugin UI to MVVM: adds ShellSettingViewModel and LeaveShellOpenOrCloseShellAfterPressEnabledConverter, migrates Settings to BaseModel with property-change notifications and localized enum values, replaces legacy code-behind UI with a data-bound CMDSetting UserControl, and adjusts Main initialization defaults and API visibility.

Changes

Cohort / File(s) Summary
Converter
Plugins/Flow.Launcher.Plugin.Shell/Converters/LeaveShellOpenOrCloseShellAfterPressEnabledConverter.cs
New public IMultiValueConverter. Validates two inputs (bool, Shell); returns enabled state when CloseShellAfterPress is false and Shell != RunCommand. ConvertBack throws NotImplementedException.
Settings model & enum localization
Plugins/Flow.Launcher.Plugin.Shell/Settings.cs
Settings now inherits BaseModel. Properties use backing fields and call OnPropertyChanged. ShowOnlyMostUsedCMDsNumber default set to 5. Shell enum annotated with [EnumLocalize] and members with [EnumLocalizeValue(...)].
ViewModel
Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs
New ShellSettingViewModel exposing Settings, localized shell list, selected shell, OnlyMostUsedCMDs numbers, and CloseShellAfterPress/LeaveShellOpen properties that enforce mutual exclusion and propagate changes to Settings.
Views (XAML)
Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml
XAML re-targeted to Flow.Launcher.Plugin.Shell.Views.CMDSetting; adds design-time DataContext, registers converter, replaces static ComboBox items with ItemsSource/bindings, binds CheckBoxes and numeric selector to ViewModel, and removes Loaded handler.
Views (code-behind)
Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml.cs
New public partial class CMDSetting : UserControl with constructor CMDSetting(Settings settings) that creates ShellSettingViewModel, sets DataContext, and calls InitializeComponent.
Legacy UI removal
Plugins/Flow.Launcher.Plugin.Shell/ShellSetting.xaml.cs
Removed previous code-behind CMDSetting that performed manual UI-event synchronization and direct Settings manipulation.
Main integration
Plugins/Flow.Launcher.Plugin.Shell/Main.cs
Added using Flow.Launcher.Plugin.Shell.Views;, ensure ShowOnlyMostUsedCMDsNumber defaults to 5 if 0, and changed API_GlobalKeyboardEvent declaration to private (accessibility change only).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant V as CMDSetting (View)
  participant VM as ShellSettingViewModel
  participant S as Settings
  participant C as LeaveShellOpenOrCloseShellAfterPressEnabledConverter

  U->>V: Toggle "CloseShellAfterPress" checkbox
  V->>VM: Bound property updated (CloseShellAfterPress)
  VM->>S: Update Settings.CloseShellAfterPress
  VM->>VM: Enforce mutual exclusion -> set LeaveShellOpen = false
  VM-->>V: PropertyChanged notifications
  V->>C: MultiBinding inputs (CloseShellAfterPress, SelectedShell)
  C-->>V: Return IsEnabled for dependent controls
Loading
sequenceDiagram
  autonumber
  participant U as User
  participant V as CMDSetting (View)
  participant VM as ShellSettingViewModel
  participant S as Settings

  U->>V: Select Shell from ComboBox
  V->>VM: SelectedShell bound value updates
  VM->>S: Settings.Shell = selected
  VM-->>V: PropertyChanged -> view updates (converter re-evaluates)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • VictoriousRaptor
  • onesounds

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title concisely and accurately summarizes the primary changes, namely refactoring the ShellSettings UI to use data binding and correcting the IsEnabled logic, clearly communicating the essence of the update without extraneous detail.
Description Check ✅ Passed The description clearly outlines the core modifications, including the switch from event handlers to binding for UI logic, the integration of localization attributes, the correction of IsEnabled behavior for specific commands, and the adjustment of the default value for ShowOnlyMostUsedCMDsNumber, all of which directly correspond to the changes in the pull request.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch shell_setting_panel_enhancement

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9be546d and 652ec40.

📒 Files selected for processing (1)
  • Plugins/Flow.Launcher.Plugin.Shell/Main.cs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Plugins/Flow.Launcher.Plugin.Shell/Main.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: gitStream workflow automation
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: build

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Shell/Main.cs (1)

226-231: Stray “/c” after pause in cmd path

pause doesn’t take /c; the trailing /c likely prints “Invalid switch - /c”. Remove it.

-                            $"{(_settings.CloseShellAfterPress ?
-                                $" && echo {notifyStr} && pause > nul /c" :
+                            $"{(_settings.CloseShellAfterPress ?
+                                $" && echo {notifyStr} && pause > nul" :
                                 "")}");
🧹 Nitpick comments (5)
Plugins/Flow.Launcher.Plugin.Shell/Converters/LeaveShellOpenOrCloseShellAfterPressEnabledConverter.cs (1)

9-19: Converter logic matches requirements

Enables only when the other option is false and shell != RunCommand. Meets “disable when RunCommand” objective.

For safer defaults during binding initialization, consider returning false instead of Binding.DoNothing so controls start disabled until values are ready.

Plugins/Flow.Launcher.Plugin.Shell/Main.cs (2)

132-136: Guard against zero/invalid history limit

If ShowOnlyMostUsedCMDsNumber is 0, users get no history. Clamp to a minimum of 1 when limiting is enabled.

Apply this diff in both places:

-            if (_settings.ShowOnlyMostUsedCMDs)
-                return [.. history.Take(_settings.ShowOnlyMostUsedCMDsNumber)];
+            if (_settings.ShowOnlyMostUsedCMDs)
+            {
+                var n = Math.Max(1, _settings.ShowOnlyMostUsedCMDsNumber);
+                return [.. history.Take(n)];
+            }

Also applies to: 185-189


66-68: Duplicate predicate in autocomplete filter

The results.Any(p => o.Equals(p.Title, ...)) check is repeated twice. Drop one for clarity.

-                                .Where(o => o.StartsWith(cmd, StringComparison.OrdinalIgnoreCase) &&
-                                            !results.Any(p => o.Equals(p.Title, StringComparison.OrdinalIgnoreCase)) &&
-                                            !results.Any(p => o.Equals(p.Title, StringComparison.OrdinalIgnoreCase))).ToList();
+                                .Where(o => o.StartsWith(cmd, StringComparison.OrdinalIgnoreCase) &&
+                                            !results.Any(p => o.Equals(p.Title, StringComparison.OrdinalIgnoreCase))).ToList();
Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml.cs (1)

10-14: Remove redundant DataContext assignment

DataContext = viewModel; is set twice. Keep one (before InitializeComponent is fine if XAML bindings depend on it).

-            DataContext = viewModel;
-            InitializeComponent();
-            DataContext = viewModel;
+            DataContext = viewModel;
+            InitializeComponent();
Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs (1)

74-77: Fail fast on null settings
Please guard the constructor argument so we fail fast with a descriptive ArgumentNullException instead of running into harder-to-debug null dereferences later.

-    public ShellSettingViewModel(Settings settings)
-    {
-        Settings = settings;
-    }
+    public ShellSettingViewModel(Settings settings)
+    {
+        Settings = settings ?? throw new ArgumentNullException(nameof(settings));
+    }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b6ea73 and d106c51.

📒 Files selected for processing (7)
  • Plugins/Flow.Launcher.Plugin.Shell/Converters/LeaveShellOpenOrCloseShellAfterPressEnabledConverter.cs (1 hunks)
  • Plugins/Flow.Launcher.Plugin.Shell/Main.cs (1 hunks)
  • Plugins/Flow.Launcher.Plugin.Shell/Settings.cs (2 hunks)
  • Plugins/Flow.Launcher.Plugin.Shell/ShellSetting.xaml.cs (0 hunks)
  • Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs (1 hunks)
  • Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml (2 hunks)
  • Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml.cs (1 hunks)
💤 Files with no reviewable changes (1)
  • Plugins/Flow.Launcher.Plugin.Shell/ShellSetting.xaml.cs
🧰 Additional context used
🧬 Code graph analysis (3)
Plugins/Flow.Launcher.Plugin.Shell/Settings.cs (1)
Plugins/Flow.Launcher.Plugin.Sys/Settings.cs (2)
  • Settings (6-127)
  • Settings (8-14)
Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml.cs (1)
Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs (2)
  • ShellSettingViewModel (5-78)
  • ShellSettingViewModel (74-77)
Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs (1)
Plugins/Flow.Launcher.Plugin.Shell/Main.cs (4)
  • List (29-97)
  • List (99-136)
  • List (163-189)
  • List (440-480)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: gitStream.cm
  • GitHub Check: build
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.Shell/Settings.cs (3)

6-20: INotifyPropertyChanged migration looks good

Adopting BaseModel with change notifications across settings is consistent and enables clean bindings.


64-76: RunAsAdministrator default is true — confirm intent

Defaulting to elevation changes behavior and UX. If previous default was false, this is a notable change. Please confirm this is intentional for existing users/settings migrations.


131-145: Enum localization annotations — LGTM

Attributes on Shell enum align with the new localization pipeline.

Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml (2)

41-47: IsEnabled MultiBinding + converter — LGTM

Correctly disables both options when RunCommand is selected and enforces mutual exclusivity at the UI level.

Also applies to: 55-61


81-84: Bindings for Shell and “Most Used” number — solid

Data-driven shells with SelectedValuePath=Value is clean. The number ComboBox wiring is correct; once Settings defaults to a valid value (e.g., 5), selection will populate as expected.

If you keep 0 as a persisted value from older configs, consider normalizing it on load to 5 to avoid a blank selection.

Also applies to: 95-97

Jack251970 and others added 2 commits September 30, 2025 09:37
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link

gitstream-cm bot commented Sep 30, 2025

This PR contains a TODO statement. Please check to see if they should be removed.

@Jack251970 Jack251970 merged commit 7f0851b into dev Sep 30, 2025
16 checks passed
@Jack251970 Jack251970 deleted the shell_setting_panel_enhancement branch September 30, 2025 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Code Quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants